Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Dereference arel_table aliases between requests #18673

Closed
wants to merge 1 commit into from
Closed

Dereference arel_table aliases between requests #18673

wants to merge 1 commit into from

Conversation

avit
Copy link
Contributor

@avit avit commented Jan 24, 2015

Repeatedly calling Model.arel_table.alias pushed additional state into the memoized class variable that never got cleared.

Because Arel::Table has mutable state (which I assume is needed for alias tracking) the memoized instance should not persist across different threads.

@avit
Dereference arel_table aliases between requests
5759708 
Repeatedly calling `Model.arel_table.alias` pushed additional state into
the memoized class variable that never got cleared.
@sgrif
Copy link
Contributor

sgrif commented Jan 24, 2015

arel_table is not part of the public API

@sgrif sgrif closed this Jan 24, 2015
@avit
Copy link
Contributor Author

avit commented Jan 24, 2015

The failing tests might be due to the class variable @arel_table getting referenced elsewhere, so this patch might be too naïve just yet.

@avit
Copy link
Contributor Author

avit commented Jan 24, 2015

That's too bad @sgrif I suspect arel_table gets used a lot whether it's "public" or not.

This is still a subtle vector for a memory leak, so I'd like to resolve it if we could...

I just had a look through the ActiveRecord source and arel_table.alias is only ever called once, in a test. Why then, does the arel_table.aliases array exist, since ActiveRecord has its own alias tracker and seemingly doesn't use that from Arel::Table? (Maybe patching Arel to make it immutable is the better fix?)

@avit
Copy link
Contributor Author

avit commented Jan 24, 2015

Heh. http://robots.thoughtbot.com/using-arel-to-compose-sql-queries

:snark:

@rails rails locked and limited conversation to collaborators Jan 25, 2015
@avit avit mentioned this pull request Jan 6, 2017
Closed
@rails rails unlocked this conversation Jan 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@avit @sgrif